Improve std::sys::windows::compat #76979

m-ou-se · 2020-09-20T14:12:59Z

Improves the compat_fn macro in sys::windows, which is used for conditionally loading APIs that might not be available.

The module (dll) name can now be any string, not just an ident. (Not all Windows api modules are valid Rust identifiers. E.g. WaitOnAddress comes from API-MS-Win-Core-Synch-l1-2-0.dll.)
Adds FuncName::is_available() for checking if a function is really available without having to do a duplicate lookup.
Add comment explaining the lack of locking.
Use $_:block to simplify the macro_rules.
Apply allow(unused_variables) only to the fallback instead of everything.

The second point (is_available()) simplifies code that needs to pick an implementation depening on what is available, like sys/windows/mutex.rs. Before this change, it'd do its own lookup and keep its own AtomicUsize to track the result. Now it can just use c::AcquireSRWLockExclusive::is_available() directly.

This will also be useful when park/unpark/CondVar/etc. get improved implementations (e.g. from parking_lot or something else), as the best APIs for those are not available before Windows 8.

rust-highfive · 2020-09-20T14:13:02Z

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

dtolnay · 2020-10-01T11:31:02Z

library/std/src/sys/windows/compat.rs

-            mem::transmute::<usize, F>(addr)($($argname),*)
+            #[allow(dead_code)]
+            pub fn is_available() -> bool {
+                addr() != fallback as usize


Function pointer equality is not well defined, since we give functions to LLVM as unnamed_addr which means their address is claimed to not be significant to the execution of the program. See #54685. This expression can evaluate to unequal even if lookup failed, and can evaluate to equal even if lookup succeeded. Is there a different way to implement is_available, or is there some reason to believe this wouldn't hit bizarre behavior?

Oh, interesting. Thanks.

If I understand correctly, unnamed_addr allows for merging functions. Meaning that f1 == f2 might give true for different functions, but not that f1 == f1 might give false for the same function. So false positives, but not false negatives. That would be fine here. The functions loaded from GetProcAddress are definitely not merged with fallback (since they come from external dll), and that's the only one that matters. If multiple fallback functions happen to have the same address for example, that's fine.

So the question here is whether 'the address is not significant' means 'not significant but at least constant', or that it may assumef != f.

Disabling unnamed_addr on fallback would also work, but it seems there's no way to do that in Rust.

Ah, @eddyb tells me rustc itself might duplicate functions too. So, in this snippet it could mean that fallback as usize (which is short for fallback as fn() as usize) might result in a copy of fallback. Using a static FALLBACK: fn(..) = fallback; and using that static instead should prevent that. The assumption that that works is already made in core:

rust/library/core/src/fmt/mod.rs

Lines 246 to 267 in 9cba260

// This guarantees a single stable value for the function pointer associated with

// indices/counts in the formatting infrastructure.

//

// Note that a function defined as such would not be correct as functions are

// always tagged unnamed_addr with the current lowering to LLVM IR, so their

// address is not considered important to LLVM and as such the as_usize cast

// could have been miscompiled. In practice, we never call as_usize on non-usize

// containing data (as a matter of static generation of the formatting

// arguments), so this is merely an additional check.

//

// We primarily want to ensure that the function pointer at `USIZE_MARKER` has

// an address corresponding *only* to functions that also take `&usize` as their

// first argument. The read_volatile here ensures that we can safely ready out a

// usize from the passed reference and that this address does not point at a

// non-usize taking function.

#[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")]

static USIZE_MARKER: fn(&usize, &mut Formatter<'_>) -> Result = |ptr, _| {

// SAFETY: ptr is a reference

let _v: usize = unsafe { crate::ptr::read_volatile(ptr) };

loop {}

};

See also #54685 (comment)

I'll update the PR to do the same.

Rebased and added 63b6007 to fix this issue.

FWIW this looks fine to me now.

- Module name can now be any string, not just an ident. (Not all Windows api modules are valid Rust identifiers.) - Adds c::FuncName::is_available() for checking if a function is really available without having to do a duplicate lookup. - Add comment explaining the lack of locking. - Use `$_:block` to simplify the macro_rules. - Apply allow(unused_variables) only to the fallback instead of everything.

dtolnay · 2020-10-01T17:36:00Z

@bors r+

bors · 2020-10-01T17:36:02Z

📌 Commit 63b6007 has been approved by dtolnay

…llback-check, r=dtolnay Improve std::sys::windows::compat Improves the compat_fn macro in sys::windows, which is used for conditionally loading APIs that might not be available. - The module (dll) name can now be any string, not just an ident. (Not all Windows api modules are valid Rust identifiers. E.g. `WaitOnAddress` comes from `API-MS-Win-Core-Synch-l1-2-0.dll`.) - Adds `FuncName::is_available()` for checking if a function is really available without having to do a duplicate lookup. - Add comment explaining the lack of locking. - Use `$_:block` to simplify the macro_rules. - Apply `allow(unused_variables)` only to the fallback instead of everything. --- The second point (`is_available()`) simplifies code that needs to pick an implementation depening on what is available, like `sys/windows/mutex.rs`. Before this change, it'd do its own lookup and keep its own `AtomicUsize` to track the result. Now it can just use `c::AcquireSRWLockExclusive::is_available()` directly. This will also be useful when park/unpark/CondVar/etc. get improved implementations (e.g. from parking_lot or something else), as the best APIs for those are not available before Windows 8.

Rollup of 11 pull requests Successful merges: - rust-lang#76851 (Fix 'FIXME' about using NonZeroU32 instead of u32.) - rust-lang#76979 (Improve std::sys::windows::compat) - rust-lang#77111 (Stabilize slice_ptr_range.) - rust-lang#77147 (Split sys_common::Mutex in StaticMutex and MovableMutex.) - rust-lang#77312 (Remove outdated line from `publish_toolstate` hook) - rust-lang#77362 (Fix is_absolute on WASI) - rust-lang#77375 (rustc_metadata: Do not forget to encode inherent impls for foreign types) - rust-lang#77385 (Improve the example for ptr::copy) - rust-lang#77389 (Fix some clippy lints) - rust-lang#77399 (BTreeMap: use Unique::from to avoid a cast where type information exists) - rust-lang#77429 (Link `new` method in `DefautHasher`s doc) Failed merges: r? `@ghost`

rust-highfive assigned withoutboats Sep 20, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 20, 2020

This comment has been minimized.

Sign in to view

rustbot added O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Sep 20, 2020

dtolnay requested changes Oct 1, 2020

View reviewed changes

m-ou-se added 3 commits October 1, 2020 16:08

Use AcquireSRWLockExclusive::is_available() instead of an extra lookup.

93310ef

Formatting.

09cbaf4

m-ou-se force-pushed the windows-fallback-check branch from aefcd26 to eca1661 Compare October 1, 2020 14:50

Work around potential merging/duplication issues in sys/windows/compat.

63b6007

m-ou-se force-pushed the windows-fallback-check branch from eca1661 to 63b6007 Compare October 1, 2020 14:52

dtolnay approved these changes Oct 1, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 1, 2020

JohnTitor mentioned this pull request Oct 1, 2020

Rollup of 11 pull requests #77436

Merged

Dylan-DPC-zz mentioned this pull request Oct 1, 2020

Rollup of 14 pull requests #77437

Closed

bors merged commit 00b3450 into rust-lang:master Oct 2, 2020

rustbot added this to the 1.48.0 milestone Oct 2, 2020

dtolnay assigned dtolnay and unassigned withoutboats Oct 2, 2020

m-ou-se mentioned this pull request Oct 8, 2020

Raise noncontinuable exception when aborting on earlier Windows versions #76203

Closed

m-ou-se deleted the windows-fallback-check branch September 27, 2022 07:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve std::sys::windows::compat #76979

Improve std::sys::windows::compat #76979

m-ou-se commented Sep 20, 2020

rust-highfive commented Sep 20, 2020

This comment has been minimized.

dtolnay Oct 1, 2020

m-ou-se Oct 1, 2020

m-ou-se Oct 1, 2020 •

edited

Loading

m-ou-se Oct 1, 2020 •

edited

Loading

eddyb Oct 1, 2020

dtolnay commented Oct 1, 2020

bors commented Oct 1, 2020

	// This guarantees a single stable value for the function pointer associated with
	// indices/counts in the formatting infrastructure.
	//
	// Note that a function defined as such would not be correct as functions are
	// always tagged unnamed_addr with the current lowering to LLVM IR, so their
	// address is not considered important to LLVM and as such the as_usize cast
	// could have been miscompiled. In practice, we never call as_usize on non-usize
	// containing data (as a matter of static generation of the formatting
	// arguments), so this is merely an additional check.
	//
	// We primarily want to ensure that the function pointer at `USIZE_MARKER` has
	// an address corresponding only to functions that also take `&usize` as their
	// first argument. The read_volatile here ensures that we can safely ready out a
	// usize from the passed reference and that this address does not point at a
	// non-usize taking function.
	#[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")]
	static USIZE_MARKER: fn(&usize, &mut Formatter<'_>) -> Result = \|ptr, _\| {
	// SAFETY: ptr is a reference
	let _v: usize = unsafe { crate::ptr::read_volatile(ptr) };
	loop {}
	};

Improve std::sys::windows::compat #76979

Improve std::sys::windows::compat #76979

Conversation

m-ou-se commented Sep 20, 2020

rust-highfive commented Sep 20, 2020

This comment has been minimized.

dtolnay Oct 1, 2020

Choose a reason for hiding this comment

m-ou-se Oct 1, 2020

Choose a reason for hiding this comment

m-ou-se Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

m-ou-se Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

eddyb Oct 1, 2020

Choose a reason for hiding this comment

dtolnay commented Oct 1, 2020

bors commented Oct 1, 2020

m-ou-se Oct 1, 2020 •

edited

Loading

m-ou-se Oct 1, 2020 •

edited

Loading